Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature test usart into release #12

Merged
merged 6 commits into from May 3, 2019
Merged

Conversation

PatrickDekker98
Copy link
Contributor

No description provided.

test/Makefile Outdated
@@ -9,10 +9,12 @@
#############################################################################

# source files in this project (main.cpp is automatically assumed)
SOURCES := $(wildcard ../code/src/*.cpp)
#SOURCES := $(wildcard ../code/src/*.cpp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gone by 85fdce9



public:

test_usart_c(unsigned int baudrate, uart_ports_c usart_port);
test_usart_c();

/// @brief does not actualy disable anything
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is wrong btw. The function name is enable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edited in 85fdce9

@@ -47,5 +48,17 @@ namespace r2d2 {
///@return unsigned int always 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't always return 1 anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edited in 85fdce9

}
}

void test_usart_c::set_receive_byte(const uint8_t byte){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function sounds like the byte i set here will be the only byte that will be in the receive_buffer. Maybe a name change to something like add_receive_byte() is better here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or a clear before the push is oke as well i guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was changed in 85fdce9

return receive_buffer.copy_and_pop();
}

bool test_usart_c::char_available() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between char_available() and available()? seems a bit redundant they return the exact same atm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

char_available returns true if char is available, available returns the amound of bytes available

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But both char_available() and available() return !receive_buffer.empty() that is unintended then i guess

#include <test_usart.hpp>
#include <uart_ports.hpp>

TEST_CASE("test_usart_c sends","[test_usart_c]"){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a test with the set_receive_string() and the set_receive_bytes()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

were added in 85fdce9

Copy link
Contributor

@itzandroidtab itzandroidtab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty nice to test the usart bus with actual data. Got some small points above.

Copy link
Contributor

@itzandroidtab itzandroidtab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except the char_available() and the available() both return !receive_buffer.empty() and your comment suggests that available() should return a amount of data left

@PatrickDekker98 PatrickDekker98 merged commit a3470c1 into release May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants